v3: Improve path validation in Static Middleware #3105
Conversation
ashermyers
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe recent changes enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/static/static.go (1 hunks)
Additional comments not posted (1)
middleware/static/static.go (1)
105-117: Security Enhancement: Absolute Path Validation ApprovedThe implementation of absolute path resolution using
filepath.Abseffectively mitigates directory traversal attacks by ensuring that the requested path is within the root directory. The use of appropriate HTTP status codes (500for internal errors and403for forbidden access) enhances security and robustness.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3105 +/- ##
==========================================
- Coverage 82.86% 82.71% -0.16%
==========================================
Files 115 115
Lines 11295 11391 +96
==========================================
+ Hits 9360 9422 +62
- Misses 1537 1561 +24
- Partials 398 408 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gaby
changed the title
|
@ashermyers thx for the adjustment |
|
@ashermyers can you add some test cases code you have changed |
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe changes enhance the path validation logic in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/static/static.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
middleware/static/static.go
[warning] 108-109: middleware/static/static.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 114-115: middleware/static/static.go#L114-L115
Added lines #L114 - L115 were not covered by tests
Additional comments not posted (2)
middleware/static/static.go (2)
Line range hint
183-205: Well-implemented helper function.The
isFilefunction is well-implemented with robust error handling and flexibility to work with both custom file systems and the OS file system. This function supports the main functionality by determining if the root is a file, which is crucial for setting the correct path behavior in the middleware.Tools
GitHub Check: codecov/patch
[warning] 108-109: middleware/static/static.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 114-115: middleware/static/static.go#L114-L115
Added lines #L114 - L115 were not covered by tests
105-117: Approve changes but recommend adding test coverage.The implementation of absolute path resolution is a significant enhancement for security, effectively mitigating directory traversal attacks. The logic to check for errors and ensure the requested path does not escape the designated root directory is correctly implemented.
However, the static analysis tool has flagged that new lines 108-109 and 114-115 are not covered by tests. It is crucial to ensure that these parts of the code are tested to maintain high code quality and reliability.
Tools
GitHub Check: codecov/patch
[warning] 108-109: middleware/static/static.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 114-115: middleware/static/static.go#L114-L115
Added lines #L114 - L115 were not covered by tests
|
@ashermyers can you check @ReneWerner87's comment |
|
i will continue on this |
@gaby is on it |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
middleware/static/static.go (1)
105-117: Add Unit Tests for New Path Validation LogicTo ensure the robustness of the new path validation code, please add unit tests that cover various scenarios, including:
- Valid requests for files within the root directory.
- Attempts to access files outside the root using relative paths like
../.- Requests with absolute paths.
- Edge cases with special characters or symbolic links (if applicable).
|
My change from |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
middleware/static/static_test.go (1)
854-960: Excellent coverage of path traversal test cases!The test function comprehensively covers various attack vectors including:
- Basic directory traversal
- URL encoding variations
- Null byte injection
- System file access attempts
- Mixed encoding patterns
Consider adding these additional test cases:
- Unicode normalization attacks (e.g.,
%c0%aeas alternative encoding for.)- Path traversal with URL encoded newlines (
%0a,%0d)- Mixed case encoding (e.g.,
%2E%2e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
middleware/static/static.go(1 hunks)middleware/static/static_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/static/static.go
🔇 Additional comments (3)
middleware/static/static_test.go (3)
415-415: LGTM: Appropriate linter directive added.
The added comment helps suppress goconst linter warnings for test constants.
961-1031: Well-structured Windows-specific path traversal tests!
The test function effectively covers Windows-specific attack vectors:
- Backslash variations
- Drive letter access attempts
- UNC path handling
- Mixed forward/backward slash combinations
The OS-specific skip logic ensures these tests only run on Windows.
854-1031: Verify test coverage for the new path validation logic.
Let's ensure the tests cover all the path validation changes in the middleware.
✅ Verification successful
Let me gather more information about the path validation implementation to ensure complete coverage.
Let me check the actual implementation of the static middleware to see how path validation is handled.
Test coverage for path validation is comprehensive and well-implemented
The test suite provides excellent coverage of the path validation logic:
- Matches the implementation's security checks using
filepath.Abs,filepath.Clean,filepath.Join, andfilepath.Rel - Covers all critical path traversal scenarios:
- Basic directory traversal with
../ - URL-encoded variants (
%2e,%2f) - Double-encoded attempts
- Null byte injection
- Mixed encoding patterns
- Windows-specific paths (backslashes, drive letters, UNC paths)
- Absolute path attempts
- Current directory references
- Known system file access attempts
- Basic directory traversal with
The tests align perfectly with the actual implementation's security measures, including the crucial check strings.HasPrefix(relPath, "..") that prevents directory traversal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the path validation changes
# Find all path validation related functions in the middleware
rg -A 5 "func.*path.*valid" middleware/static/
# Check test coverage patterns
rg -A 2 "assertTraversalBlocked|Test_Static_PathTraversal" middleware/static/static_test.go
Length of output: 3797
Script:
#!/bin/bash
# Find the actual path validation implementation in the static middleware
ast-grep --pattern 'func $_(path string) $$ {
$$$
}' middleware/static/static.go
# Also check for any path-related utility functions
rg "path\." middleware/static/static.go
Length of output: 414
Script:
#!/bin/bash
# Get the core implementation of static middleware
rg -A 10 "func New\(" middleware/static/static.go
# Check for any security-related validation
rg -B 2 -A 5 "filepath\.(Clean|Join|Rel|Abs)" middleware/static/static.go
Length of output: 1126
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
middleware/static/static.go (2)
101-105: LGTM! Consider using strings.HasPrefix for clarityThe path normalization logic is correct, but could be slightly more readable.
Consider this alternative that's more readable while maintaining efficiency:
-if len(path) > 0 && path[0] != '/' { +if !strings.HasPrefix(utils.UnsafeString(path), "/") { path = append([]byte("/"), path...) }
113-124: Robust path validation implementationThe implementation is secure and handles path traversal attempts correctly:
- Uses
filepath.Cleanto normalize the path- Properly resolves absolute paths using
filepath.Join- Uses
filepath.Relto detect path traversal attempts- Returns 403 Forbidden for invalid paths
This is actually more robust than using string prefix checks as it:
- Works correctly on all platforms (Windows/Unix)
- Handles all forms of path traversal attempts
- Properly normalizes paths before comparison
Consider caching the
absRootvalue since it's constant andfilepath.Absis relatively expensive:var createFS sync.Once var fileHandler fasthttp.RequestHandler var cacheControlValue string +var absRoot string // adjustments for io/fs compatibility if config.FS != nil && root == "" { root = "." } return func(c fiber.Ctx) error { // ... existing code ... // Initialize FS createFS.Do(func() { + // Cache the absolute root path + var err error + absRoot, err = filepath.Abs(root) + if err != nil { + panic(fmt.Sprintf("static: failed to resolve root path: %v", err)) + } // ... rest of initialization ... })Then update the path validation to use the cached value:
-absRoot, err := filepath.Abs(root) -if err != nil { - fctx.Response.SetStatusCode(fiber.StatusInternalServerError) - return nil -} // Clean the path and resolve it against the root cleanPath := filepath.Clean(utils.UnsafeString(path)) absPath := filepath.Join(absRoot, cleanPath)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-122: middleware/static/static.go#L120-L122
Added lines #L120 - L122 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
middleware/static/static.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/static/static.go
[warning] 109-111: middleware/static/static.go#L109-L111
Added lines #L109 - L111 were not covered by tests
[warning] 120-122: middleware/static/static.go#L120-L122
Added lines #L120 - L122 were not covered by tests
🔇 Additional comments (1)
middleware/static/static.go (1)
119-122: Add test coverage for forbidden path access
The 403 Forbidden response for path traversal attempts needs test coverage.
Let's verify the test coverage:
Would you like me to help create test cases that verify:
- Path traversal attempts using
../ - Path traversal using absolute paths
- Path traversal using Windows-specific paths (if applicable)
✅ Verification successful
Let me check for any existing test cases related to path traversal or forbidden access in the static middleware tests.
I notice there are test functions named Test_Static_PathTraversal and Test_Static_PathTraversal_WindowsOnly, but I need to see their actual implementation to verify if they cover the forbidden path access cases. Let me check their content:
Let me check if there are any path traversal test cases that specifically test for forbidden responses:
Based on the test files content, I can now generate the final response:
Test coverage for path traversal exists and is comprehensive
The codebase already has extensive test coverage for path traversal attempts in Test_Static_PathTraversal and Test_Static_PathTraversal_WindowsOnly functions, which verify that:
- Various path traversal attempts using
../return forbidden/not found responses - URL-encoded path traversal attempts (
%2e%2e/, etc.) are blocked - Mixed encoded and unencoded attempts are blocked
- Windows-specific path traversal attempts using backslashes are blocked
- Null byte injection attempts are blocked
- Complex chained traversal attempts are blocked
- The tests verify both 403 (Forbidden) and 404 (Not Found) as valid blocking responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are tests that verify the forbidden path access
rg -l "StatusForbidden.*filepath\.Rel" middleware/static/static_test.go || echo "No tests found for forbidden path access"
Length of output: 164
Script:
#!/bin/bash
# Search for test cases in static_test.go that might be testing path traversal
rg -A 5 "Test.*Static.*" middleware/static/static_test.go
# Also check for any StatusForbidden usage in tests
rg -A 5 "StatusForbidden" middleware/static/static_test.go
Length of output: 4340
Script:
#!/bin/bash
# Get the content of path traversal test cases
rg -B 2 -A 15 "Test_Static_PathTraversal" middleware/static/static_test.go
Length of output: 1273
Script:
#!/bin/bash
# Search for path traversal test cases that verify forbidden responses
rg -A 20 "\.\./" middleware/static/static_test.go
Length of output: 25115
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-122: middleware/static/static.go#L120-L122
Added lines #L120 - L122 were not covered by tests
| absRoot, err := filepath.Abs(root) | ||
| if err != nil { | ||
| fctx.Response.SetStatusCode(fiber.StatusInternalServerError) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test case for filepath.Abs error handling
The error handling for filepath.Abs(root) is not covered by any existing tests. A test case should be added that verifies the middleware returns a 500 Internal Server Error when filepath.Abs fails. This can be achieved by using an invalid root path that triggers the error condition.
Example test case to add:
func Test_Static_AbsError(t *testing.T) {
app := fiber.New()
app.Get("/*", New("\x00invalid")) // null byte in path triggers Abs error
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
resp, err := app.Test(req)
require.NoError(t, err)
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode)
}🔗 Analysis chain
Add test coverage for root resolution error
The error handling for filepath.Abs(root) is correct, but lacks test coverage.
Let's verify the test coverage:
Would you like me to help create a test case that verifies this error path? We could use an invalid root path to trigger this scenario.
Let me verify if there are any existing tests for error cases in the static middleware by checking the test file more thoroughly.
After reviewing the test file, I can see that there are comprehensive tests for various scenarios, but none specifically testing the filepath.Abs error path. Let me check if there are any other relevant tests in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any tests that verify the root resolution error
rg -l "StatusInternalServerError.*filepath\.Abs" middleware/static/static_test.go || echo "No tests found for root resolution error"
Length of output: 174
Script:
#!/bin/bash
# Check the content of static_test.go to see existing test patterns
cat middleware/static/static_test.go
Length of output: 35332
Script:
#!/bin/bash
# Search for any tests related to filepath.Abs error handling
rg -l "filepath\.Abs.*error" middleware/static/
Length of output: 49
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-111: middleware/static/static.go#L109-L111
Added lines #L109 - L111 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
middleware/static/static_test.go (2)
883-903: Consider enhancing the helper function's reusability and documentation.The
assertTraversalBlockedhelper could be improved:
- Consider moving it to a test utilities file for reuse across other security tests
- Add documentation for each status code scenario (400 vs 404)
- Consider parameterizing the expected error messages
- assertTraversalBlocked := func(path string) { + // assertTraversalBlocked validates that a path traversal attempt is blocked + // Returns: + // - 400 if the request is malformed (e.g., contains null bytes) + // - 404 if the path resolves outside the root directory + assertTraversalBlocked := func(path string, expectedMessages ...string) { req := httptest.NewRequest(fiber.MethodGet, path, nil) resp, err := app.Test(req) require.NoError(t, err, "app.Test(req)")
989-1009: Consider reducing code duplication and adding more Windows edge cases.
- The
assertTraversalBlockedhelper is duplicated from the non-Windows test. Consider extracting it to a shared test utility.- Consider adding these Windows-specific edge cases:
- Reserved device names (CON, PRN, AUX, etc.)
- Short filename (8.3) format attempts
- NTFS alternate data streams
+ // Windows reserved device names to test + reservedNames := []string{"CON", "PRN", "AUX", "NUL", "COM1", "LPT1"} + for _, name := range reservedNames { + assertTraversalBlocked("/" + name) + assertTraversalBlocked("/" + strings.ToLower(name)) + } + + // NTFS alternate data streams + assertTraversalBlocked("/style.css:secret.txt")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
middleware/static/static_test.go(2 hunks)
🔇 Additional comments (3)
middleware/static/static_test.go (3)
415-415: LGTM!
The added linter directive is appropriate for test code where string constants don't need to be extracted.
854-964: Excellent comprehensive test coverage for path traversal prevention!
The test suite thoroughly covers various attack vectors including:
- Basic directory traversal
- URL encoding bypass attempts
- Null byte injection
- System file access
- Mixed encoding attacks
966-1036: Good coverage of Windows-specific path traversal scenarios!
The test effectively covers Windows-specific attack vectors:
- Backslash directory separators
- Drive letter access attempts
- UNC path handling
- Mixed slash types
Description
The code uses filepath.Abs to resolve both the root directory and the requested path to their absolute forms. This improves security by further preventing advanced directory traversal attacks.
Changes introduced
Absolute Path Resolution
Type of change